Allow Implicit RID Opt Out + Don't Infer RID for non EXE Type Projects #28628

nagilson · 2022-10-17T23:42:43Z

Description

Allows UseCurrentRuntimeIdentifier property to opt out of this feature (Implicit RIDS) in case someone doesn't want it
Resolves issue where you do the following:

Create a .NET Core project (e,g, Winforms APP)
In same solution, add a Class Library and set it as a project reference to the main project (created in step1)
Navigate to ClickOnce publish wizard, and set all as default
Navigate to Configurations wizard and set it to Framework-Dependent, SingleFile, x86

Publish and see failure. Only fails in VS. Also, if you set the properties anywhere besides VS, it won't fail.

Before:

After:

Why?

PublishSingleFile (PSF) is not supposed to work if you don't set a RID, but it works if you set a RID in the top-level project but then don't flow the RID to the project references.
AppendsRuntimeIdentifierToOutputPath is set to true even with there is no rid in a library if the top level project had a rid, so the library is ridless but tries to append it's nonexistent rid to the output path which used to be OK
We tried to infer a RID because PSF generally requires a RID to exist but the check for the RID doesn't run in PrepareForPublish for Non-Exe projects.
When VS Injects their own publishing properties in the publishing wizard the pathing gets mixed up.

So, we can solve this by only doing that inference for Exe OutputTypes since PSF doesn't require a RID in subprojects. Rainer confirmed with me that it fixed this scenario. We need to take it to tactics now most likely.

cc @rainersigwald @dsplaisted

Customer Impact

Will allow customers to correctly publish libraries in the VS Publishing wizard but also keep the feature of not having to specify a RID when building or publishing selfcontained and friends.

Regression

This is a new feature so not a regression.

Risk

Low because it only lowers the scope of a newer feature and allows opting out of that feature instead of doing it everytime.
But there is a chance it might break something else we haven't thought of. So far, we didn't find anything else.

Testing

See the above Before and after screenshot.

dotnet-issue-labeler · 2022-10-17T23:42:50Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

rainersigwald · 2022-10-18T14:04:25Z

There is another possible solution that might help some other scenarios (like the exe-depends-on-exe we talked about): stop PublishSingleFile (and related properties) from flowing to ProjectReferences. However, that's a bigger, riskier change, and I like the focused scope of this fix.

rainersigwald · 2022-10-18T14:05:48Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets

@@ -65,6 +65,7 @@ Copyright (c) .NET Foundation. All rights reserved.
                 (
                 '$(RuntimeIdentifier)' == '' and
                 '$(RuntimeIdentifiers)' == '' and
+                 '$(OutputType)' == 'Exe' and


Is it possible to also add an explicit do-not-infer-RID property here? Since it's caused trouble once, it would be nice to be able to offer a solution to users who hit another related problem without requiring servicing.

You probably want to use _IsExecutable or HasRuntimeOutput instead of OutputType here.

Making the change, but for my own understanding: may you please explain why either of those is better than OutputType?

The OutputType can also be WinExe, which these properties take into account.

dsplaisted

Is it possible to add a test for the failing scenario?

dsplaisted · 2022-10-18T14:28:34Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets

@@ -65,6 +65,7 @@ Copyright (c) .NET Foundation. All rights reserved.
                 (
                 '$(RuntimeIdentifier)' == '' and
                 '$(RuntimeIdentifiers)' == '' and
+                 '$(OutputType)' == 'Exe' and


You probably want to use _IsExecutable or HasRuntimeOutput instead of OutputType here.

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets

…imeIdentifierInference.targets Co-authored-by: Rainer Sigwald <[email protected]>

…nto nagilson-exe-rids

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets

…s Exe Co-authored-by: Daniel Plaisted <[email protected]>

nagilson · 2022-10-18T21:48:16Z

cc @baronfel @richlander to potentially comment on how bad or not bad it would be if we removed implicit RIDs for 7.0, in case there is objection to adding this fix and tactics would rather pull the feature for now. We're bringing this to tactics because we found a scenario where this broke VS publishing; VS publishing has their own RID logic. This fixed it but can't be 100% sure there isn't any additional stuff we haven't thought of. Preferably, we keep it.

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets

src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishASingleFileLibrary.cs

Initial Feature was added in 2020 but not documented here: dotnet/sdk#10449 We've added implicit RIDs here: dotnet/sdk#26143 And we've added interaction to toggle implicit RIDs off with UCR here: dotnet/sdk#28628 And we added the shorthand for ucr here: dotnet/sdk#27971 Needs to be duplicated for `dotnet publish` as well.

Don't infer rid on non exe project type

bcaed25

nagilson added the Bug label Oct 17, 2022

rainersigwald reviewed Oct 18, 2022

View reviewed changes

dsplaisted reviewed Oct 18, 2022

View reviewed changes

Add opt out feature and tests

a80d6b5

nagilson changed the title ~~Don't Infer RID for non EXE Type Projects~~ Allow Implicit RID Opt Out + Don't Infer RID for non EXE Type Projects Oct 18, 2022

rainersigwald reviewed Oct 18, 2022

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets Outdated Show resolved Hide resolved

nagilson and others added 4 commits October 18, 2022 11:54

Update src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Runt…

10c3bc5

…imeIdentifierInference.targets Co-authored-by: Rainer Sigwald <[email protected]>

Make it a win only fact

47c024b

Merge branch 'nagilson-exe-rids' of https://github.com/nagilson/sdk i…

0f654d6

…nto nagilson-exe-rids

make the test fail in the old scenario

fb14de3

dsplaisted reviewed Oct 18, 2022

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets Outdated Show resolved Hide resolved

Consider that TestProjects May be Exe Projects without Being Tagged a…

d3ed325

…s Exe Co-authored-by: Daniel Plaisted <[email protected]>

nagilson mentioned this pull request Oct 18, 2022

[7.0.2xx] Change Global Property Flow to Not Flow Publish* Properties #28645

Open

nagilson marked this pull request as ready for review October 18, 2022 21:33

nagilson added the Servicing-consider label Oct 18, 2022

nagilson mentioned this pull request Oct 18, 2022

RuntimeIdentifiers Code Coverage + Implicit RuntimeIdentifiers (not just runtimeIdentifier) #28484

Merged

dsplaisted reviewed Oct 19, 2022

View reviewed changes

nagilson added 3 commits October 19, 2022 12:03

Remove opt out feature and put it into UCR

b256814

Fix tests. Only thing remaining is to improve test quality

d1c8744

Fix whitespace

a00b2cf

rainersigwald approved these changes Oct 19, 2022

View reviewed changes

nagilson enabled auto-merge October 19, 2022 22:30

Fix tests

d3173a2

nagilson merged commit 6942fe7 into dotnet:release/7.0.1xx Oct 20, 2022

nagilson mentioned this pull request Oct 20, 2022

Improve Tests for Implicit RIDs and Libraries #28696

Merged

nagilson mentioned this pull request Oct 21, 2022

Document UseCurrentRuntime dotnet/docs#31970

Merged

nagilson mentioned this pull request Jan 9, 2023

NativeAOT publish for DLL does not set automatically RID #29834

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Implicit RID Opt Out + Don't Infer RID for non EXE Type Projects #28628

Allow Implicit RID Opt Out + Don't Infer RID for non EXE Type Projects #28628

nagilson commented Oct 17, 2022 •

edited

Loading

dotnet-issue-labeler bot commented Oct 17, 2022

rainersigwald commented Oct 18, 2022

rainersigwald Oct 18, 2022

dsplaisted Oct 18, 2022

nagilson Oct 18, 2022

dsplaisted Oct 18, 2022

dsplaisted left a comment

dsplaisted Oct 18, 2022

nagilson commented Oct 18, 2022 •

edited

Loading

Allow Implicit RID Opt Out + Don't Infer RID for non EXE Type Projects #28628

Allow Implicit RID Opt Out + Don't Infer RID for non EXE Type Projects #28628

Conversation

nagilson commented Oct 17, 2022 • edited Loading

Description

Customer Impact

Regression

Risk

Testing

dotnet-issue-labeler bot commented Oct 17, 2022

rainersigwald commented Oct 18, 2022

rainersigwald Oct 18, 2022

Choose a reason for hiding this comment

dsplaisted Oct 18, 2022

Choose a reason for hiding this comment

nagilson Oct 18, 2022

Choose a reason for hiding this comment

dsplaisted Oct 18, 2022

Choose a reason for hiding this comment

dsplaisted left a comment

Choose a reason for hiding this comment

dsplaisted Oct 18, 2022

Choose a reason for hiding this comment

nagilson commented Oct 18, 2022 • edited Loading

nagilson commented Oct 17, 2022 •

edited

Loading

nagilson commented Oct 18, 2022 •

edited

Loading